Refactor and update QR Op #1518
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jessegrabowski
added
enhancement
jessegrabowski
force-pushed
the
qr-shape-inference
branch
3 times, most recently
from
5bc044c to
be949cd
Compare
ricardoV94
reviewed
Jul 3, 2025
ricardoV94
reviewed
Jul 3, 2025
jessegrabowski
force-pushed
the
qr-shape-inference
branch
4 times, most recently
from
71639ef to
a6d6c11
Compare
jessegrabowski
force-pushed
the
qr-shape-inference
branch
from
a6d6c11 to
112f6fd
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (49.75%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1518 +/- ##
==========================================
- Coverage 81.85% 81.45% -0.41%
==========================================
Files 230 232 +2
Lines 52522 53027 +505
Branches 9345 9422 +77
==========================================
+ Hits 42992 43192 +200
- Misses 7095 7390 +295
- Partials 2435 2445 +10
🚀 New features to boost your workflow:
|
ricardoV94
reviewed
Jul 15, 2025
| in_dtype = config.floatX if integer_input else dtype | ||
|
|
||
| @numba_njit(cache=False) | ||
| def qr(a): |
There was a problem hiding this comment.
Do we need to worry about a not being F-contiguous like other lapack/blas stuff?
There was a problem hiding this comment.
I see, it's done in the lower level functions
ricardoV94
approved these changes
Jul 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates the QRFull Op, adding static shape checking, infer_shape, and destroy_map. It also optimizes the perform method for the C backend, and tries to improve the gradient graph by checking static shapes (to avoid an ifelse).
I renamed it to QR, because I don't know what was Full about the old one. I also moved it from the numpy implementation to scipy, which gives us all the usual benefits (inplace, etc). I also went ahead and unpacked the scipy wrapper and used the LAPACK functions directly. This will give us better error handling (that is to say, none -- it should eventually return a matrix of NaN on failure) and some performance boost by caching workspace requirements.
Still a WIP, because it breaks everything by moving QR from nlinalg to slinalg. I thought about using this as an opportunity to finally eliminate this distinction and go to a more logical organization (linalg/decomposition/qr.py), but then decided against it for now. Needs discussion.
Related Issue
infer_shapemethod toQRFull#1511Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1518.org.readthedocs.build/en/1518/